-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle write-only properties #1377
Conversation
- After create and update, merge the create-only inputs as outputs for completeness and to avoid SDKs filling in the non-checked inputs. - Don't create-only output props if they happen to be returned - prefer the values from the service where possible even if we weren't expecting them. - Warn about missing properties on import and refresh.
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1377 +/- ##
==========================================
- Coverage 22.82% 22.71% -0.11%
==========================================
Files 25 25
Lines 4197 4226 +29
==========================================
+ Hits 958 960 +2
- Misses 3079 3105 +26
- Partials 160 161 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from the Go test setup question
baseGo := base.With(integration.ProgramTestOptions{ | ||
Verbose: true, | ||
Dependencies: []string{ | ||
"github.com/pulumi/pulumi-aws-native/sdk", | ||
fmt.Sprintf("github.com/pulumi/pulumi-aws-native/sdk=%s", sdkPath), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this shouldn't be necessary, and I'm surprised it even works. ProgramTest already creates the mod replace
entries for each of the Dependencies
here.
In pulumi-aws, e.g., we just pass the name of the dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That code only works if the PULUMI_DEP_ROOT env is set, so fails for running tests in the IDE. This is a much more reliable approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. It would be more concise then to have a ProgramTest option equivalent to PULUMI_DEP_ROOT, and just pass in the plain dependencies to be replaced.
Fixes #1373
This also allows us to re-enable testing of refreshes.